Support dynamic port for `c:Endpoint.url/0` #5553

LostKobrakai · 2023-08-16T09:16:52Z

This is a naive implementation, as it doesn't yet support the adapter crashing and potentially getting assigned a new port. I'm not sure if cowboy or bandit for the matter crash all the way up to the endpoint in such a case, so phoenix could react to a new port or if there would need to be some other mechanism to make phoenix aware of the changed port.

That should still be better than leaving the :0, as it works right now, I think.

Relates to #4186

mtrudel

Happy to coordinate changes as needed on mtrudel/bandit#212 to add any necessary functions to the Bandit adapter to keep this PR simple.

mtrudel · 2023-08-16T13:39:54Z

lib/phoenix/endpoint/supervisor.ex

-        https -> {"https", https[:port] || 443}
-        http -> {"http", http[:port] || 80}
+        https -> {"https", transport_port(endpoint, https, :https)}
+        http -> {"http", transport_port(endpoint, http, :http)}


Do we need to be so explicit about sorting through the config ourselves? Rather than special casing the times where the port is dynamic, we could just always ask the adapter for the port (I'm happy to add dynamic_port/2 to Bandit.PhoenixAdapter) and be done with it. Taken with the changes suggested for domain sockets above, it would strip away most of the complexity of the changes to this file, I think.

That sounds good, but it also kinda prepends on the adapter being able to return a port / being started. That should be a simpler check though.

That's a good point - at many of the calls we're guaranteed that the server won't be running. What ought to be the behaviour in those cases, do you think?

I think if the server is not running the current behaviour (of rendering :0 as port) is already the best one. Phoenix builds up a url it expects the server to bind to. That behaviour just doesn't help when a server is actually started and urls need to be build to connect to that server. In that case phoenix should be able to provide the port the server was bound to instead.

This means querying the information after startup and at best also make sure that cached information stays up to date with the bound port in case of crashes/restarts or the suspend you mentioned.

mtrudel · 2023-08-16T13:43:51Z

lib/phoenix/endpoint/cowboy2_adapter.ex

+    case :ranch.get_addr(build_ref(endpoint, scheme)) do
+      {:local, _unix_path} -> raise "Trying to determine dynamic port, but the adapter is configured to use unix sockets. Remove the configuration for `[port: 0]` on the #{scheme} scheme."


I would suggest plumbing through a value of nil for the port in the case of domain sockets; it's better than falling back to the scheme port as we're doing below.

Regardless, it's an unwinnable battle either way I think.

Hmm, I saw it the other way round. If you're using unix socket nothing should even ask for a port, as that doesn't really make sense. Given the above discussion returning nil might be more appropriate though.

Good point. If we step back and observe that the goal here is to build a URI for the server, we should be asking 'what does that URI even look like for a domain socket' (and secondarily, 'is that the URI that the user cares about').

We have answers to this already in Bandit and Cowboy. If we wanted to follow down that path we should be extracting both the address and the port from the underlying server (that makes sense to me, FWIW).

mtrudel · 2023-08-16T13:49:33Z

I'm not sure if cowboy or bandit for the matter crash all the way up to the endpoint in such a case, so phoenix could react to a new port or if there would need to be some other mechanism to make phoenix aware of the changed port.

Crashes of specific handlers won't unwind all of Bandit or Cowboy, so we're fine there. What may happen though, is changes to the port number in the case of suspension (see Thousand Island docs and Ranch docs, 'Suspending and resuming a listener').

josevalim · 2023-08-24T11:07:57Z

Yeah, I don't think we should do this, especially if url will change between compile vs runtime. We use this in Livebook and we basically compute and store our own URL based on port: 0. :)

LostKobrakai · 2023-08-24T11:17:05Z

I've been running into this with elixir desktop, which currently integrates with cowboy specifically partly due to this. I'm fine if this might not be a responsibility for phoenix, but I'd also love to see a shared solution to the issue rather than a "figure it out on your own". Especially with the printed banner on startup it really feels strange that this would be the only place in phoenix knowing the actual port, with no other way to get to know it across all adapters.

Edit: Seems like livebook also queries :ranch directly using what feels like phoenix integration details (the ranch references used).

mtrudel · 2023-08-24T13:23:57Z

Yeah, I don't think we should do this, especially if url will change between compile vs runtime. We use this in Livebook and we basically compute and store our own URL based on port: 0. :)

Building URLs for a server is hard.

Setting aside issues of constructing URLs that account for load balancers, domain sockets, etc, even the simple case above is more nuanced than what José mentions, since the recent 'suspend/resume' patterns that @chrismccord et al have been working on will end up changing the port between resumptions. Ultimately it's really difficult to build a URL that has guaranteed meaning beyond the scope of a specific connection and as encoded already within a Plug.Conn. And even that isn't perfect, since the port & host that we see is often not the one that the user sees (as I've talked about before).

This isn't an isolated problem, either. There's currently a discussion going on over on uberauth that closely relates to this, and similar issues have come up a few times in Bandit's history. I do think that there's an opportunity to encapsulate all of this within the adapter layer, as e.g. LiveView having to know about ranch is a less than ideal encapsulation of the whole thing. I think we would want to:

Formalize exactly what the port, host, etc, fields in a Plug.Conn mean for connection information in the context of a specific connection (see previous discussion on this here)
Provide a way for servers to be able to provide information about their bound host and port in a general sense (ie: not with relation to a specific connection). My thought here would be as part of Plug, so that we could implement it via the Plug.Conn.Adapter behaviour.
More ambitiously, encapsulate server startup & lifecycle this way as well. This would alleviate the need for server-specific phoenix adapters, and also provide a single place for URL construction both for programatically built URLs as well as user configured ones (José, we've spoked about this in the past regarding URL construction as part of the Plug.Conn.init/5 API).

Specifically with respect to this issue, it sounds like the appetite is to leave it alone for now. If we can find consensus on a broader approach that would solve this and issues such as the one on uberauth at the moment, I'm happy to drive implementation.

WDYT?

josevalim · 2023-08-24T13:39:21Z

Making it a responsibility of the connection makes a lot of sense. For desktop apps, there aren’t proxies, so always accurate. We do have rewrite plugs for proxies too.

LostKobrakai · 2023-08-24T13:58:37Z

Provide a way for servers to be able to provide information about their bound host and port in a general sense

This is what I feel is the missing piece – being able to initially connect to the dynamic port without depending on implementation details about the underlying servers. Any later route building based on an actual connection could be handled with plug/on_mount tooling to make sure url(@conn_or_socket, ~p"…") works (or even just route with relative paths only).

Point 3. certainly sounds ambitious. I'm wondering if there's an in between solution, where the server implementation could inform phoenix when their bound interface has changed.

josevalim · 2023-09-14T18:54:25Z

We could augment the Plug.Adapter protocol to introduce a info function with metadata about the server but I'd say this is out of scope of Phoenix (or at least until it is tackled in Plug). :) Thanks @LostKobrakai!

mtrudel · 2023-09-14T19:13:05Z

I'm planning on tackling a PR against Plug et al. for this in the next couple of weeks!

josevalim · 2023-11-02T13:40:08Z

This has been added to main.

LostKobrakai added 3 commits August 16, 2023 10:54

Resolve port 0 to dynamic port when server is started

d1e7923

Add check for callback

3c4ded3

Deduplicate endpoint module

cf5911f

LostKobrakai mentioned this pull request Aug 16, 2023

Add Bandit.PhoenixAdapter.bandit_pid/2 mtrudel/bandit#212

Merged

LostKobrakai marked this pull request as ready for review August 16, 2023 09:35

mtrudel suggested changes Aug 16, 2023

View reviewed changes

mtrudel mentioned this pull request Aug 17, 2023

fix: callback_url/1 uses conn.port by default ueberauth/ueberauth#187

Merged

josevalim closed this Sep 14, 2023

This was referenced Oct 31, 2023

Upgrade to bandit livebook-dev/livebook#2316

Merged

Add c:Phoenix.Endpoint.server_info/1 #5620

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support dynamic port for `c:Endpoint.url/0` #5553

Support dynamic port for `c:Endpoint.url/0` #5553

LostKobrakai commented Aug 16, 2023 •

edited

Loading

mtrudel left a comment

mtrudel Aug 16, 2023

LostKobrakai Aug 16, 2023

mtrudel Aug 16, 2023

LostKobrakai Aug 16, 2023

mtrudel Aug 16, 2023

LostKobrakai Aug 16, 2023

mtrudel Aug 16, 2023

mtrudel commented Aug 16, 2023

josevalim commented Aug 24, 2023

LostKobrakai commented Aug 24, 2023 •

edited

Loading

mtrudel commented Aug 24, 2023

josevalim commented Aug 24, 2023 •

edited

Loading

LostKobrakai commented Aug 24, 2023

josevalim commented Sep 14, 2023

mtrudel commented Sep 14, 2023

josevalim commented Nov 2, 2023

		case :ranch.get_addr(build_ref(endpoint, scheme)) do
		{:local, _unix_path} -> raise "Trying to determine dynamic port, but the adapter is configured to use unix sockets. Remove the configuration for `[port: 0]` on the #{scheme} scheme."

Support dynamic port for c:Endpoint.url/0 #5553

Support dynamic port for c:Endpoint.url/0 #5553

Conversation

LostKobrakai commented Aug 16, 2023 • edited Loading

mtrudel left a comment

Choose a reason for hiding this comment

mtrudel Aug 16, 2023

Choose a reason for hiding this comment

LostKobrakai Aug 16, 2023

Choose a reason for hiding this comment

mtrudel Aug 16, 2023

Choose a reason for hiding this comment

LostKobrakai Aug 16, 2023

Choose a reason for hiding this comment

mtrudel Aug 16, 2023

Choose a reason for hiding this comment

LostKobrakai Aug 16, 2023

Choose a reason for hiding this comment

mtrudel Aug 16, 2023

Choose a reason for hiding this comment

mtrudel commented Aug 16, 2023

josevalim commented Aug 24, 2023

LostKobrakai commented Aug 24, 2023 • edited Loading

mtrudel commented Aug 24, 2023

josevalim commented Aug 24, 2023 • edited Loading

LostKobrakai commented Aug 24, 2023

josevalim commented Sep 14, 2023

mtrudel commented Sep 14, 2023

josevalim commented Nov 2, 2023

Support dynamic port for `c:Endpoint.url/0` #5553

Support dynamic port for `c:Endpoint.url/0` #5553

LostKobrakai commented Aug 16, 2023 •

edited

Loading

LostKobrakai commented Aug 24, 2023 •

edited

Loading

josevalim commented Aug 24, 2023 •

edited

Loading